refactor(asusctl): modularize CLI handlers into dedicated modules and fix service status guards - #238
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR modularizes Changesasusctl command handlers
D-Bus and structured logging support
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
coderabbitai
Bot
added
asusctl
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
asusctl/src/main.rs (1)
102-108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
conn: &Connectionover suppressing the lint.
do_parsedonly ever passes&connto handlers, so taking the borrow removes the need for#[allow(clippy::needless_pass_by_value)].🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@asusctl/src/main.rs` around lines 102 - 108, Update do_parsed to accept conn as &Connection instead of Connection, remove the #[allow(clippy::needless_pass_by_value)] attribute, and preserve the existing handler calls by passing the borrowed connection through.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@asusctl/src/anime_cli.rs`:
- Around line 205-291: The argument checks in the anime and platform handlers
run inside per-device loops, causing repeated warnings and partial updates
before returning. In asusctl/src/anime_cli.rs lines 205-291, hoist the image,
pixel-image, gif, pixel-gif path.is_empty() checks and SetBuiltins
builtins.set.is_none() check before the for proxy in animes loop, returning
before proxy work; in asusctl/src/platform_cli.rs lines 344-348, move the
power.command let Some(...) else validation before the for aura in aura loop and
remove the empty “Commands available” print.
- Around line 170-177: Update the anime type selection around get_anime_type so
cmd.override_type is applied unconditionally whenever provided, regardless of
whether detection returns Unsupported. Retain the warning only for an
Unsupported detected type when no override is supplied.
In `@asusctl/src/platform_cli.rs`:
- Line 72: Move the rog_dbus::find_iface_blocking import from its current
location into the file’s existing top-level import block, without changing its
usage.
- Around line 289-300: Update the loop over AuraProxyBlocking devices to track
whether any eligible device was handled instead of returning immediately from
the first match. Call handle_led_power_1_do_1866 for every old or TUF laptop,
then emit the existing warning only after iteration when no matching device was
found, preserving the function’s successful return behavior.
- Around line 512-561: The handle_armoury_command function repeats the armoury
interface lookup in each subcommand arm. Resolve attrs once before matching on
cmd.command, then reuse it in List, Get, and Set; optionally extract the shared
Get/Set attribute search while preserving their distinct read versus mutation
behavior.
- Around line 482-496: Update the possible-values output block around the `p.0`
checks to always emit a terminating newline after printing the closing `]`,
before the `has_default` branch prints `default:` or the blank line. Preserve
the existing comma and closing-bracket formatting.
- Around line 118-135: Construct a single PlatformProxyBlocking instance before
the match on cmd.command, then reuse that proxy in the Limit, OneShot, and Info
arms. Remove the repeated PlatformProxyBlocking::new(conn)? calls while
preserving each arm’s existing operations and error propagation.
In `@asusctl/src/scsi_cli.rs`:
- Around line 46-47: Update the scsi command flow around find_iface_blocking and
the cmd.list handling so --list is processed before D-Bus interface discovery
and succeeds without asusd or the SCSI interface. Only call find_iface_blocking
when device options are supplied, while preserving existing behavior for
device-specific operations.
In `@asusctl/src/slash_cli.rs`:
- Around line 82-83: Update the no-argument branch in the slash command handler
to return immediately after emitting the missing-argument warning. Ensure the
early return occurs before opening the system connection or constructing the
Slash proxy, while preserving the existing usage message.
---
Outside diff comments:
In `@asusctl/src/main.rs`:
- Around line 102-108: Update do_parsed to accept conn as &Connection instead of
Connection, remove the #[allow(clippy::needless_pass_by_value)] attribute, and
preserve the existing handler calls by passing the borrowed connection through.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 50e31e4d-5cc3-4278-bb3c-8ae52471e7fa
📒 Files selected for processing (9)
asusctl/src/anime_cli.rsasusctl/src/fan_curve_cli.rsasusctl/src/main.rsasusctl/src/platform_cli.rsasusctl/src/scsi_cli.rsasusctl/src/slash_cli.rsrog-control-center/src/main.rsrog-control-center/src/mocking.rsrog-control-center/src/zbus_proxies.rs
📜 Review details
🔇 Additional comments (15)
rog-control-center/src/zbus_proxies.rs (2)
1-5: LGTM!Also applies to: 71-89
91-91: 🎯 Functional CorrectnessNo compatibility alias needed. No live Rust callers still reference
find_iface; the only remaining occurrence is a commented example.> Likely an incorrect or invalid review comment.rog-control-center/src/mocking.rs (1)
5-6: LGTM!Also applies to: 98-125, 137-137, 152-154, 165-165, 198-202
rog-control-center/src/main.rs (1)
83-91: LGTM!asusctl/src/anime_cli.rs (3)
2-5: LGTM!Also applies to: 35-37
295-325: LGTM!Also applies to: 327-375
199-203: 🎯 Functional CorrectnessUse a zeroed buffer for
clear.AnimeDataBuffer::new()starts with0u8, and the other clear path also sendsvec![0u8; ...];255would light the whole panel.> Likely an incorrect or invalid review comment.asusctl/src/fan_curve_cli.rs (1)
2-2: LGTM!Also applies to: 46-119
asusctl/src/platform_cli.rs (5)
1-19: LGTM!Also applies to: 21-70
74-112: LGTM!Also applies to: 140-217
219-276: LGTM!Also applies to: 306-333, 350-388
390-434: LGTM!
436-481: LGTM!Also applies to: 500-510
asusctl/src/main.rs (1)
1-19: LGTM!Also applies to: 95-100, 117-131
asusctl/src/slash_cli.rs (1)
2-2: LGTM!Also applies to: 142-149
8196d47 to
7998c1a
Compare
7998c1a to
95f3669
Compare
95f3669 to
1015b1f
Compare
6e0aaef to
0acfc9e
Compare
…ync into rog-dbus
…le values newline
0acfc9e to
dbd9a0e
Compare
coderabbitai
Bot
added
asusd
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
rog-control-center/src/zbus_proxies.rs (1)
91-126: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the shared blocking interface helper.
rog_dbus::find_iface_blockingnow implements this operation. This local copy already differs in its log level and its no-interface error text.Re-export the shared helper with
find_iface_async. Remove this duplicate implementation.Proposed change
-pub use rog_dbus::find_iface_async; - -pub fn find_iface_blocking<T>(iface_name: &str) -> Result<Vec<T>, Box<dyn std::error::Error>> -where - T: zbus::blocking::proxy::ProxyImpl<'static> + From<zbus::Proxy<'static>>, -{ - // local implementation -} +pub use rog_dbus::{find_iface_async, find_iface_blocking};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rog-control-center/src/zbus_proxies.rs` around lines 91 - 126, Replace the local find_iface_blocking implementation with a re-export of rog_dbus::find_iface_blocking alongside find_iface_async. Remove the duplicate connection, interface lookup, logging, sorting, proxy construction, and no-interface error-handling code from this module.asusctl/src/main.rs (1)
101-107: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTake
connby reference instead of suppressing the lint.
do_parsednever consumesconn; every use at lines 115-126 borrows it. Change the parameter to&Connectionand remove theallowattribute.♻️ Proposed refactor
-#[allow(clippy::needless_pass_by_value)] fn do_parsed( parsed: &CliStart, supported_interfaces: &[String], supported_properties: &[Properties], - conn: Connection, + conn: &Connection, ) -> Result<(), Box<dyn std::error::Error>> {Update the call site at line 83:
if let Err(err) = do_parsed(&parsed, &supported_interfaces, &supported_properties, &conn) {Then replace
&connwithconnin the handler calls at lines 115, 116, 124, and 126.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@asusctl/src/main.rs` around lines 101 - 107, Update do_parsed to accept conn as &Connection and remove the clippy::needless_pass_by_value suppression. Pass &conn at its call site, then pass conn directly to each handler invocation inside do_parsed, preserving the existing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@asusctl/src/anime_cli.rs`:
- Around line 195-198: Preserve the presence validation in the
AnimeActions::SetBuiltins branch, but use the contained builtins.set boolean
when reaching the proxy.set_builtin_animations apply path. Only invoke the proxy
operation when the value is true; treat Some(false) as a valid no-op rather than
applying the change unconditionally.
- Around line 152-201: Move the per-action validation block keyed by
cmd.command.as_ref() and matching AnimeActions variants before
find_iface_blocking and get_anime_type in handle_anime. Preserve all existing
warnings and early returns for empty image, pixel-image, gif, pixel-gif paths,
and missing SetBuiltins.set, so invalid arguments are rejected before any D-Bus
discovery or anime-type lookup.
In `@asusctl/src/main.rs`:
- Around line 116-129: Update the command dispatch around handle_slash_get,
handle_slash_set, and xgm_led_cli::handle_xgm_led to pass the existing &conn,
and change those handler signatures and their call sites to accept and reuse the
shared Connection instead of opening another system bus connection. Preserve the
current SlashSubCommand behavior and error propagation while applying the same
shared-connection pattern used by fan_curve_cli::handle_fan_curve and
handle_battery.
- Around line 95-98: Update the CLI logging initialization around the existing
filter_level and env_logger target configuration to use an info-level default
filter while still honoring explicit RUST_LOG settings, and keep the logger
output on stdout rather than configuring env_logger::Target::Stderr. Ensure the
info! records in main remain visible as primary command results and continue
supporting pipelines and stdout redirection.
In `@asusctl/src/platform_cli.rs`:
- Around line 546-555: Update the attribute handling around the local name
binding to reuse the result from attr.name() when checking property_type(),
eliminating the second name() D-Bus call. Ensure the value remains available
after the comparison with s.property, using a reference or clone if the
conversion moves it, and preserve the existing FirmwareAttributeType::Ppt
behavior.
- Around line 41-51: Update check_systemd_unit_active to return true only when
systemctl is-active produces the exact active state, trimming the stdout before
comparison. Treat activating, deactivating, unknown, inactive, failed, command
errors, and other outputs as false.
- Around line 464-469: Update the match in the current-value reporting logic to
handle (None, Some(c), None) by logging c as the current value instead of
reporting it unavailable. Preserve the existing formatting for all other
min/c/max combinations.
- Around line 515-521: Remove the #[allow(clippy::manual_is_multiple_of,
clippy::nonminimal_bool)] attribute immediately above handle_armoury_command,
leaving the function signature and implementation unchanged so both Clippy lints
are enforced.
- Around line 309-331: Add a validation in the aura power command before
constructing LaptopAuraPower or calling aura.set_led_power so an empty states
collection is rejected when no --keyboard or --lightbar zone was selected. Emit
a clear warning or error and return an appropriate failure result, while
preserving the existing behavior when at least one zone is selected.
- Around line 289-296: Update the device-type branching around
handle_led_power_1_do_1866 so the warning is emitted when the handler does not
apply, using the same predicate for both warning and handling without evaluating
it twice. Preserve handle_led_power_1_do_1866 and handled = true only for old or
TUF laptops, and ensure the warning describes that supported scope.
In `@asusctl/src/scsi_cli.rs`:
- Around line 56-98: Update the SCSI device-processing closure around
led_mode_data and set_led_mode_data to handle each device’s errors locally, log
the device context, and continue processing remaining devices instead of
propagating failures through ?. Adjust the closure return type and error
handling to match the actual operation signatures. Move the “first 4 colours”
warning out of the scsis loop so it is emitted only once.
In `@asusd/src/daemon.rs`:
- Around line 40-41: Update the two warn! messages in the daemon startup
guidance to correct the spelling of “should” and the grammar in the journalctl
instruction, while preserving the existing service and command references.
In `@rog-control-center/src/lib.rs`:
- Around line 25-33: Ensure the --version path in main remains visible when
RUST_LOG is unset by emitting the version information directly to stdout or
initializing that path with an info-level filter. Update the version-reporting
block containing the rog-gui, asusd, and component crate version messages while
preserving the existing output content.
---
Outside diff comments:
In `@asusctl/src/main.rs`:
- Around line 101-107: Update do_parsed to accept conn as &Connection and remove
the clippy::needless_pass_by_value suppression. Pass &conn at its call site,
then pass conn directly to each handler invocation inside do_parsed, preserving
the existing behavior.
In `@rog-control-center/src/zbus_proxies.rs`:
- Around line 91-126: Replace the local find_iface_blocking implementation with
a re-export of rog_dbus::find_iface_blocking alongside find_iface_async. Remove
the duplicate connection, interface lookup, logging, sorting, proxy
construction, and no-interface error-handling code from this module.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: aa159c36-b7ad-4823-87d8-8bf8211379da
📒 Files selected for processing (17)
asus-shutdown/src/main.rsasusctl/src/anime_cli.rsasusctl/src/fan_curve_cli.rsasusctl/src/main.rsasusctl/src/platform_cli.rsasusctl/src/scsi_cli.rsasusctl/src/slash_cli.rsasusctl/src/xgm_led_cli.rsasusd/src/daemon.rsrog-anime/src/data.rsrog-aura/src/keyboard/layouts.rsrog-control-center/src/lib.rsrog-control-center/src/main.rsrog-control-center/src/zbus_proxies.rsrog-dbus/Cargo.tomlrog-dbus/src/lib.rssimulators/src/simulator.rs
📜 Review details
🔇 Additional comments (29)
rog-dbus/Cargo.toml (1)
20-20: LGTM!rog-dbus/src/lib.rs (1)
76-76: LGTM!Also applies to: 96-129
rog-control-center/src/zbus_proxies.rs (1)
71-89: LGTM!rog-control-center/src/main.rs (1)
86-91: LGTM!asus-shutdown/src/main.rs (1)
155-171: LGTM!asusd/src/daemon.rs (1)
32-38: LGTM!Also applies to: 42-42
rog-anime/src/data.rs (1)
7-7: LGTM!Also applies to: 320-320
rog-aura/src/keyboard/layouts.rs (1)
308-308: LGTM!Also applies to: 474-474
simulators/src/simulator.rs (1)
121-121: LGTM!asusctl/src/platform_cli.rs (8)
23-39: LGTM!
65-110: LGTM!
112-134: LGTM!
136-171: LGTM!
173-213: LGTM!
215-272: LGTM!
334-386: LGTM!
388-444: LGTM!asusctl/src/main.rs (2)
4-19: LGTM!
48-48: LGTM!Also applies to: 64-64
asusctl/src/slash_cli.rs (1)
2-2: LGTM!Also applies to: 82-83, 139-160
asusctl/src/xgm_led_cli.rs (1)
11-19: LGTM!asusctl/src/anime_cli.rs (3)
2-5: LGTM!Also applies to: 9-37, 42-48, 56-79, 83-96, 113-132, 140-151
203-295: 🗄️ Data Integrity & IntegrationConfirm behavior when
find_iface_blockingreturns more than one proxy.
for proxy in animes(line 203) applies image/GIF actions per device. ForGif/PixelGif, the media is decoded viafrom_gif/from_pnginside this loop (lines 260-283), so with multiple proxies the same file gets redecoded once per device. More importantly,play_gif_animation(lines 308-328) blocks the calling thread onproxy, and whengif.loops == 0it loops forever (None => continue). If more than one Anime proxy is ever returned, only the first device receives the animation and the command never advances to the rest ofanimes.This may be a non-issue if each supported laptop model always exposes exactly one Anime D-Bus object (in which case the loop is effectively single-iteration), but
rog_dbus::find_iface_blockingis shared, multi-device discovery infrastructure per the PR's "Blocking D-Bus discovery" layer. Confirm whetherfind_iface_blockingcan return more than oneAnimeProxyBlocking, and if so, hoist the media decode out of the loop and avoid blocking subsequent devices on an infinite GIF loop.#!/bin/bash # Description: Inspect find_iface_blocking's signature and behavior in rog-dbus. fd -e rs . rog-dbus/src | xargs rg -n -B2 -A20 'fn find_iface_blocking'Also applies to: 308-328
298-306: LGTM!Also applies to: 330-338, 340-378
asusctl/src/fan_curve_cli.rs (4)
2-2: LGTM!Also applies to: 46-48
69-72: 🩺 Stability & AvailabilityVerify that
FanCurvesProxyBlocking::new()actually detects service unavailability.This handler builds the proxy directly with
FanCurvesProxyBlocking::new(conn)and maps any error to a "Fan curves unavailable" warning. The sibling handlers inplatform_cli.rsandanime_cli.rs, and this file's neighborscsi_cli.rs, all userog_dbus::find_iface_blockingto discover the interface before calling it. If the derived blocking proxynew()does not itself validate that the destination interface exists on the bus (a common zbus proxy behavior, where errors only surface on the first method call), thismap_errbranch never triggers, and the intended "service availability guard" silently does not work. Confirm the proxy-construction semantics forFanCurvesProxyBlockingand align this handler with thefind_iface_blockingpattern used elsewhere if construction alone does not validate availability.#!/bin/bash set -euo pipefail fd -a 'lib.rs' rog-dbus echo "---- find_iface_blocking definition ----" rg -n -A 25 'pub fn find_iface_blocking' rog-dbus/src/lib.rs || true echo "---- FanCurvesProxyBlocking definition/macro ----" fd -a 'fan_curves' rog-dbus rg -n -B3 -A 20 'FanCurvesProxyBlocking|dbus_proxy' rog-dbus/src/zbus_fan_curves.rs 2>/dev/null || true
83-86: LGTM!
88-115: LGTM!asusctl/src/scsi_cli.rs (1)
34-41: LGTM!
| log::info!("App and daemon versions:"); | ||
| log::info!(" rog-gui v{}", VERSION); | ||
| log::info!(" asusd v{}", asusd::VERSION); | ||
| log::info!("\nComponent crate versions:"); | ||
| log::info!(" rog-anime v{}", rog_anime::VERSION); | ||
| log::info!(" rog-aura v{}", rog_aura::VERSION); | ||
| log::info!(" rog-dbus v{}", rog_dbus::VERSION); | ||
| log::info!(" rog-profiles v{}", rog_profiles::VERSION); | ||
| log::info!(" rog-platform v{}", rog_platform::VERSION); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep --version visible with the default log filter.
main sets RUST_LOG to warn,... when it is unset. That filter suppresses every log::info! call here. As a result, rog-control-center --version produces no version output by default.
Keep this command output on stdout, or initialize the version path with an info log filter.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rog-control-center/src/lib.rs` around lines 25 - 33, Ensure the --version
path in main remains visible when RUST_LOG is unset by emitting the version
information directly to stdout or initializing that path with an info-level
filter. Update the version-reporting block containing the rog-gui, asusd, and
component crate version messages while preserving the existing output content.
…and connection sharing
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
rog-control-center/src/zbus_proxies.rs (1)
71-90: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftAdd or remove the GPU D-Bus proxy.
asusddoes not declare or registerxyz.ljones.Gpu; only this proxy defines the interface. Add the interface at/xyz/ljones/Gpuwith the threeStringproperties, or remove the proxy. Any call to it currently fails at runtime.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rog-control-center/src/zbus_proxies.rs` around lines 71 - 90, The GpuStatus proxy references an unregistered asusd interface, so either register xyz.ljones.Gpu at /xyz/ljones/Gpu with power_status, vendor, and mode String properties in the asusd D-Bus implementation, or remove GpuStatus and its callers from zbus_proxies.rs; ensure no runtime path still invokes an unavailable interface.asusctl/src/anime_cli.rs (1)
229-282: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winValidate media brightness before any proxy write.
If a command combines a global setting or
--clearwith a media action, Lines 204-227 can change a device before Lines 232, 246, 258, or 273 rejectbright. The error exits the loop and leaves later devices unchanged. Validate all media brightness values beforefind_iface_blockingand beforefor proxy in animes.Proposed fix
if let Some(action) = cmd.command.as_ref() { match action { AnimeActions::Image(image) if image.path.is_empty() => { warn!("Missing arg or command; run 'asusctl anime image --help' for usage"); return Ok(()); } AnimeActions::PixelImage(image) if image.path.is_empty() => { warn!("Missing arg or command; run 'asusctl anime pixel-image --help' for usage"); return Ok(()); } AnimeActions::Gif(gif) if gif.path.is_empty() => { warn!("Missing arg or command; run 'asusctl anime gif --help' for usage"); return Ok(()); } AnimeActions::PixelGif(gif) if gif.path.is_empty() => { warn!("Missing arg or command; run 'asusctl anime pixel-gif --help' for usage"); return Ok(()); } AnimeActions::SetBuiltins(builtins) if builtins.set.is_none() => { warn!("Missing arg; run 'asusctl anime set-builtins --help' for usage"); return Ok(()); } + AnimeActions::Image(image) => verify_brightness(image.bright)?, + AnimeActions::PixelImage(image) => verify_brightness(image.bright)?, + AnimeActions::Gif(gif) => verify_brightness(gif.bright)?, + AnimeActions::PixelGif(gif) => verify_brightness(gif.bright)?, _ => {} } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@asusctl/src/anime_cli.rs` around lines 229 - 282, Move brightness validation for every media action out of the per-proxy match and perform it before find_iface_blocking and the for proxy in animes loop. Ensure Image, PixelImage, Gif, and PixelGif brightness values are validated whenever present, before any global-setting or --clear operation can modify a device; remove the later duplicate validations while preserving the existing media handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@asusctl/src/main.rs`:
- Around line 26-33: Update logger initialization around the env_logger::Builder
flow to use Env::default().default_filter_or(...) with Builder::from_env,
preserving an explicitly configured global RUST_LOG filter while retaining the
existing info/tracing/zbus default and stdout target. Remove the
filter_level(LevelFilter::Info) call that overrides parsed environment
directives.
---
Outside diff comments:
In `@asusctl/src/anime_cli.rs`:
- Around line 229-282: Move brightness validation for every media action out of
the per-proxy match and perform it before find_iface_blocking and the for proxy
in animes loop. Ensure Image, PixelImage, Gif, and PixelGif brightness values
are validated whenever present, before any global-setting or --clear operation
can modify a device; remove the later duplicate validations while preserving the
existing media handling.
In `@rog-control-center/src/zbus_proxies.rs`:
- Around line 71-90: The GpuStatus proxy references an unregistered asusd
interface, so either register xyz.ljones.Gpu at /xyz/ljones/Gpu with
power_status, vendor, and mode String properties in the asusd D-Bus
implementation, or remove GpuStatus and its callers from zbus_proxies.rs; ensure
no runtime path still invokes an unavailable interface.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a73cc0ba-a8e8-4311-ab70-be9804172bf4
📒 Files selected for processing (9)
asusctl/src/anime_cli.rsasusctl/src/main.rsasusctl/src/platform_cli.rsasusctl/src/scsi_cli.rsasusctl/src/slash_cli.rsasusctl/src/xgm_led_cli.rsasusd/src/daemon.rsrog-control-center/src/main.rsrog-control-center/src/zbus_proxies.rs
📜 Review details
🔇 Additional comments (11)
asusd/src/daemon.rs (1)
23-24: LGTM!Also applies to: 32-42, 59-59
rog-control-center/src/zbus_proxies.rs (2)
1-2: LGTM!
91-91: LGTM!rog-control-center/src/main.rs (2)
30-30: LGTM!
86-91: LGTM!asusctl/src/anime_cli.rs (1)
35-36: LGTM!Also applies to: 193-201, 284-292
asusctl/src/scsi_cli.rs (1)
34-105: LGTM!asusctl/src/platform_cli.rs (1)
41-48: LGTM!Also applies to: 286-307, 464-468, 516-555
asusctl/src/slash_cli.rs (1)
70-90: LGTM!Also applies to: 126-151
asusctl/src/xgm_led_cli.rs (1)
4-22: LGTM!asusctl/src/main.rs (1)
83-83: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDispatch
scsi --listbefore the asusd preflight.
handle_scsireturns staticAuraMode::list()output before its D-Bus lookup. However, Line 83 runs only after the platform version, property, and interface checks. Ifasusdis unavailable, the version check returns first andasusctl scsi --listnever reaches the daemon-free handler. This breaks the stated daemon-freescsi --listbehavior.Proposed fix
let parsed: CliStart = argh::from_env(); + if let CliCommand::Scsi(cmd) = &parsed.command { + if cmd.list { + if let Err(err) = scsi_cli::handle_scsi(cmd) { + error!("{err}"); + } + return; + } + } + let conn = match Connection::system() {> Likely an incorrect or invalid review comment.
| if std::env::var_os("RUST_LOG").is_none() { | ||
| std::env::set_var("RUST_LOG", "warn,tracing=error,zbus=error"); | ||
| std::env::set_var("RUST_LOG", "info,tracing=error,zbus=error"); | ||
| } | ||
| let mut logger = env_logger::Builder::new(); | ||
| logger | ||
| .parse_default_env() | ||
| .filter_level(LevelFilter::Info) | ||
| .target(env_logger::Target::Stderr) | ||
| .target(env_logger::Target::Stdout) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
lockfile="$(fd -t f '^Cargo\.lock$' . | head -n 1)"
test -n "$lockfile"
version="$(
awk '
$0 == "name = \"env_logger\"" { found = 1; next }
found && /^version = / { gsub(/"/, "", $3); print $3; exit }
' "$lockfile"
)"
test -n "$version"
curl -fsSL "https://docs.rs/crate/env_logger/${version}/source/src/logger.rs" |
rg -n -C 8 'parse_env|parse_default_env|filter_level|default_filter_or'Repository: OpenGamingCollective/asusctl
Length of output: 166
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- candidate files ---'
fd -t f '^(main\.rs|Cargo\.lock|Cargo\.toml)$' . | sort
printf '%s\n' '--- logging code ---'
main="$(fd -t f '^main\.rs$' . | awk '$0=="asusctl/src/main.rs"{print; exit}')"
test -n "$main"
cat -n "$main" | sed -n '1,80p'
printf '%s\n' '--- env_logger dependency declarations ---'
rg -n -C 3 'env_logger|RUST_LOG|parse_default_env|filter_level|default_filter_or' \
--glob 'Cargo.toml' --glob 'Cargo.lock' --glob '*.rs' .Repository: OpenGamingCollective/asusctl
Length of output: 665
🌐 Web query:
env_logger Builder parse_default_env filter_level precedence filter_level source
💡 Result:
In the env_logger crate, explicit configuration methods on the Builder (such as filter_level) and environment-based configuration methods (such as parse_default_env or parse_env) can be combined. When both are used, the environment configuration overrides or appends to the existing configuration set by the builder [1][2]. Specifically, the parse_default_env and parse_env methods apply directives from the specified environment variables to the builder's internal configuration [1][2]. If you call filter_level to set an initial baseline (e.g., LevelFilter::Off) and then call parse_default_env, the directives found in the environment variable (typically RUST_LOG) will be applied, effectively overriding or refining the previously set filter [1][2]. Key points regarding precedence and behavior: - Builder methods like filter_level and filter modify the builder's state before the logger is initialized [1][2]. - Environment parsing methods like parse_default_env read from the environment and add/update those directives in the builder [1][2]. - Documentation examples explicitly show this pattern: initializing the builder, setting a baseline filter level (e.g., Off), and then calling parse_default_env to allow the environment variables to dictate the final logging behavior [1][2]. The source code for Builder::parse_default_env confirms it is a convenience wrapper that calls parse_env using the default environment settings [3]. Because these methods are applied sequentially on the mutable Builder, the order of operations dictates the final state: configuration applied last generally has the final say [1][2].
Citations:
- 1: https://docs.rs/env_logger/latest/env_logger/struct.Builder.html
- 2: https://docs.rs/env_logger/0.11.3/env_logger/struct.Builder.html
- 3: https://docs.rs/env_logger/latest/src/env_logger/logger.rs.html
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- asusctl logging code ---'
cat -n asusctl/src/main.rs | sed -n '1,75p'
printf '%s\n' '--- env_logger dependency ---'
rg -n -C 4 'env_logger' Cargo.toml asusctl/Cargo.toml
printf '%s\n' '--- lockfiles ---'
fd -t f '^Cargo\.lock$' . | sort || trueRepository: OpenGamingCollective/asusctl
Length of output: 4031
🌐 Web query:
env_logger 0.11.8 source Builder filter_level parse_default_env FilterBuilder directives precedence
💡 Result:
In env_logger 0.11.8, the Builder is used to construct a logger configuration [1][2]. Regarding your specific queries: Builder and filter_level: The filter_level(level) method adds a global directive to the logger, setting a minimum log level for all modules [1][3]. This global directive is applied to all log targets unless more specific module-level filters are provided [2][3]. parse_default_env and Precedence: The parse_default_env method is a convenience function that invokes parse_env(Env::default) [4][5]. It reads the RUST_LOG environment variable (or the default variable configured) and applies those directives to the builder [2][4]. Crucially, env_logger does not provide a built-in mechanism where "programmatic" configuration (such as calling.filter or.filter_level on the builder) automatically takes precedence over or is merged with environment variable configuration in a "soft" way [1][6]. When you call parse_env or parse_default_env, the directives found in the environment variable are added to the existing configuration [4]. If there is a conflict (e.g., you set a module to Off programmatically, but RUST_LOG enables it), the behavior depends on the order of operations [7][6]. Typically, later directives in the combined list take precedence over earlier ones [2]. Because parse_env appends to the builder's internal state, environment variables effectively act as additional filters that can override or augment your manual settings depending on the syntax used in the environment string [2][6]. Directive Precedence: env_logger uses the filter syntax defined by the env_filter crate [2][3]. When multiple directives are present, they are evaluated based on their specificity: 1. Specificity: A directive for a specific module (e.g., my_crate::module=info) takes precedence over a global directive (e.g., info) [2]. 2. Order: If multiple directives apply to the same module or if there are multiple global directives, the last one specified generally governs the behavior [2]. To ensure your programmatic filters (like setting a dependency to 'off') persist regardless of the user's RUST_LOG input, you should configure your builder such that the environment variables are parsed first or carefully structure the directive string [7][6]. Note that there is no native way to make programmatic "hard" filters that strictly override RUST_LOG without explicit logic in your code to manage that precedence [6].
Citations:
- 1: https://docs.rs/env_logger/latest/env_logger/struct.Builder.html
- 2: https://docs.rs/env_logger/
- 3: https://docs.rs/env_filter/latest/env_filter/struct.Builder.html
- 4: https://docs.rs/env_logger/latest/src/env_logger/logger.rs.html
- 5: https://docs.rs/env_logger/latest/i686-pc-windows-msvc/env_logger/struct.Builder.html
- 6: Always exclude certain dependencies from logging rust-cli/env_logger#234
- 7: [Feature Request] Default log level seanmonstar/pretty-env-logger#36
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
tmp="$(mktemp -d)"
trap 'rm -rf "$tmp"' EXIT
curl -fsSL \
'https://docs.rs/crate/env_logger/0.11.11/source/src/logger.rs' \
-o "$tmp/logger.rs"
printf '%s\n' '--- env_logger Builder methods ---'
rg -n -C 12 \
'pub fn (parse_default_env|parse_env|filter_level)|filter_level\(' \
"$tmp/logger.rs"
printf '%s\n' '--- filter implementation references ---'
rg -n -C 8 'filter_level|add_filter|filter::Builder|parse_env' "$tmp/logger.rs"Repository: OpenGamingCollective/asusctl
Length of output: 50386
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
tmp="$(mktemp -d)"
trap 'rm -rf "$tmp"' EXIT
curl -fsSL \
'https://docs.rs/crate/env_filter/0.1.4/source/src/builder.rs' \
-o "$tmp/builder.rs"
printf '%s\n' '--- env_filter directive insertion and precedence ---'
rg -n -C 12 \
'filter_level|add_filter|parse|directives|last|override' \
"$tmp/builder.rs"Repository: OpenGamingCollective/asusctl
Length of output: 1471
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
tmp="$(mktemp -d)"
trap 'rm -rf "$tmp"' EXIT
curl -fsSL \
'https://crates.io/api/v1/crates/env_logger/0.11.11/download' \
-o "$tmp/env_logger.crate"
tar -xzf "$tmp/env_logger.crate" -C "$tmp"
printf '%s\n' '--- env_logger source ---'
rg -n -A14 -B8 \
'pub fn parse_default_env|pub fn parse_env|pub fn filter_level' \
"$tmp"/env_logger-0.11.11/src/logger.rs
printf '%s\n' '--- env_logger dependency versions ---'
rg -n 'env_filter|version' "$tmp"/env_logger-0.11.11/Cargo.toml | head -n 20
printf '%s\n' '--- env_filter source ---'
curl -fsSL \
'https://crates.io/api/v1/crates/env_filter/0.1.4/download' \
-o "$tmp/env_filter.crate"
tar -xzf "$tmp/env_filter.crate" -C "$tmp"
rg -n -A18 -B8 \
'pub fn filter_level|fn filter_level|pub fn parse|fn parse|directives' \
"$tmp"/env_filter-0.1.4/srcRepository: OpenGamingCollective/asusctl
Length of output: 217
🌐 Web query:
site:docs.rs/env_filter/0.1.4 env_filter Builder filter_level source default level parse directives
💡 Result:
For env_filter 0.1.4:
Builder::filter_level(level)adds a global directive (module = None). (docs.rs)Builder::filter(module, level)applies the level to a specific module, or globally whenmoduleisNone. (docs.rs)parse("info,crate1::mod1=warn")parses comma-separated directives. Bare levels such asinfo,warn,error, andoffset the global level; module directives usemodule=level. (docs.rs)parseprints warnings and ignores invalid directives;try_parsereturnsParseError. (docs.rs)- If no directives exist when
build()runs, the default global level isError. (docs.rs) Filter::filter()returns the maximum configured level, orLevelFilter::Offif none are configured. (docs.rs)
Citations:
- 1: https://docs.rs/env_filter/0.1.4/env_filter/struct.Builder.html
- 2: https://docs.rs/env_filter/0.1.4/env_filter/struct.Builder.html
- 3: https://docs.rs/env_filter/0.1.4/src/env_filter/filter.rs.html
- 4: https://docs.rs/env_filter/0.1.4/env_filter/struct.Builder.html
- 5: https://docs.rs/env_filter/0.1.4/src/env_filter/filter.rs.html
- 6: https://docs.rs/env_filter/0.1.4/src/env_filter/filter.rs.html
🌐 Web query:
site:docs.rs/env_logger/0.11.11 "filter_level" "parse_default_env" "override"
💡 Result:
env_logger 0.11.11 documents that:
let mut builder = env_logger::Builder::new();
builder.filter_level(log::LevelFilter::Off);
builder.parse_default_env();
builder.init();parse_default_env() reads the default environment variables—primarily RUST_LOG—and allows them to override previously configured filters. It is equivalent to parse_env(Env::default()). [1][2]
The custom-variable form is:
builder.filter_level(LevelFilter::Off);
builder.parse_env("MY_LOG");Citations:
- 1: https://docs.rs/env_logger/latest/env_logger/struct.Builder.html?search=format_timestamp&utm_source=openai
- 2: https://docs.rs/crate/env_logger/latest/source/src/logger.rs?utm_source=openai
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
tmp="$(mktemp)"
trap 'rm -f "$tmp"' EXIT
curl -fsSL \
'https://docs.rs/crate/env_filter/0.1.4/source/src/filter.rs' |
python3 -c '
import html, sys
from html.parser import HTMLParser
class Text(HTMLParser):
def __init__(self):
super().__init__()
self.out = []
def handle_data(self, data):
self.out.append(data)
def get(self):
return html.unescape("".join(self.out))
p = Text()
p.feed(sys.stdin.read())
print(p.get())
' > "$tmp"
rg -n -A20 -B12 \
'struct Filter|fn enabled|directives|filter_level|sort|match|LevelFilter' \
"$tmp"Repository: OpenGamingCollective/asusctl
Length of output: 23719
Preserve an explicit global RUST_LOG filter.
filter_level(LevelFilter::Info) runs after parse_default_env() and replaces its global directive. Thus, RUST_LOG=error and RUST_LOG=debug both use global info. Use Env::default().default_filter_or(...) with Builder::from_env instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@asusctl/src/main.rs` around lines 26 - 33, Update logger initialization
around the env_logger::Builder flow to use Env::default().default_filter_or(...)
with Builder::from_env, preserving an explicitly configured global RUST_LOG
filter while retaining the existing info/tracing/zbus default and stdout target.
Remove the filter_level(LevelFilter::Info) call that overrides parsed
environment directives.
1 participant
Description
This PR modularizes the monolithic
asusctlCLI entry point into focused, single-responsibility modules, consolidates D-Bus helper functions intorog-dbusand improves daemon/CLI resilience and performance.Key Changes
CLI Modularization (
asusctl):asusctl/src/main.rs(removing ~970 lines of monolithic code) into dedicated handler modules:platform_cli.rs: Platform settings (fan profiles, CPU governor/power limits, charge thresholds, Panel OD, Armoury attributes).anime_cli.rs: AniMe Matrix display configuration and image/gif rendering handlers.fan_curve_cli.rs: Custom fan curve editing and upload subcommands.scsi_cli.rs: External SCSI LED lighting subcommands.slash_cli.rs: Slash lighting bar control subcommands.xgm_led_cli.rs: XG Mobile eGPU LED status and toggle subcommands.D-Bus Helpers Consolidation (
rog-dbus):find_iface_blockingandfind_iface_asyncproxy lookup functions acrossasusctlandrog-control-centerintorog-dbus.Structured Logging & Code Quality:
println!/eprintln!calls acrossasusctlCLI modules andasusddaemon (asusd/src/daemon.rs) to structuredlogmacros (info!,warn!,error!).asusdto the top ofmain()ensuring early startup logs are captured.Service Guards, Resilience & Performance Optimizations:
check_service) before dispatching D-Bus calls, providing friendly diagnostic output whenasusdis inactive.scsi --listexecution requirement so it can run independently of daemon status.Verification Passed
cargo check --all-targetscargo test --allcargo clippy --workspace --all-targets --all-features -- -D warningscargo crankycargo fmt --all -- --check